-
Notifications
You must be signed in to change notification settings - Fork 8
Find missing IO specializations #697
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ropagate exit failure
fef0359
to
5d6c2ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve finally looked through your changes. 😅 I’m happy with some but not all of them. I think that, since some of your changes are quite extensive, it would have been good to coordinate your work with me; the submission of your changes in a few commits added to a rebased version of my branch made it a bit hard to precisely get the results I’d like to get. I’ve now pushed some additional commits to the #683 branch, which contain some of your changes verbatim and variants of others.
@@ -0,0 +1,42 @@ | |||
#!/bin/sh | |||
|
|||
SCRIPTS_DIR="$(CDPATH= cd -- "$(dirname -- "$0")" && pwd -P)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this necessary? The script is expected to be run with the root of the repository as the current directory; after all, other linting scripts work based on the same expectation. Given this, it should be enough to just use scripts
as the scripting directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For robustness. All other linting scripts use git search, which is robust in that it searches from the repository base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, git ls-files
does not search from the repository base but from the current directory. For example, running the command git ls-files --exclude-standard --no-deleted --deduplicate '*.hs'
(taken from scripts/lint-hlint.sh
) when being in the scripts
directory yields only generate-haddock-prologue.hs
and generate-readme.hs
.
So currently several, maybe most, of our scripts have to be run with the working directory being the repository base in order to function correctly. It would be good, of course, to change this, perhaps by adding a code snippet that just changes the working directory to the repository base right at the beginning, but, since this would be a change to be made to several scripts, it should be done in a different pull request.
scripts/lint-io-specialisations.sh
Outdated
absence_allowed_file=scripts/lint-io-specialisations/absence-allowed | ||
absence_finder=scripts/lint-io-specialisations/find-absent.sh | ||
|
||
set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see no need to remove set -e
, given that it is part of POSIX and probably widely supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, it’s more robust to not rely on -e
being available? I wouldn’t believe that. The -e
option seems to be supported by basically any Unix shell: dash
has it, bash
has it, zsh
has it, and a web search hasn’t revealed any shell that would not have it.
scripts/lint-io-specialisations.sh
Outdated
) | ||
) || exit 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn’t necessary when leaving set -e
in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
# Find sed: | ||
case "$(uname)" in | ||
Darwin) | ||
sed="$(which gsed)" | ||
if [ "${sed}" = "" ]; then | ||
printf 'This script requires GNU sed, which can be installed with Homebrew:\n\n' >&2 | ||
printf ' brew install gsed\n\n' >&2 | ||
exit 1 | ||
fi | ||
;; | ||
*) | ||
sed="$(which sed)" | ||
;; | ||
esac | ||
|
||
sed=$(which gsed) | ||
if [ "${sed}" = "" ]; then | ||
sed=$(which sed) | ||
fi | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implements two, slightly different, ways of deciding on a sed executable:
gsed
on Darwin,sed
everywhere elsegsed
when available,sed
otherwise
I find solution 1 unnecessarily restrictive, since there may be other systems where GNU sed is available under the name gsed
(I remember there being at least gmake
on the Sun servers we had at university in the 1990ies 😄). Option 2 lifts that restriction, but it unconditionally uses sed
whenever there is no gsed
, even on Darwin, and thus risks picking a non-GNU sed. Best is probably to use gsed
whenever available, use sed
if gsed
is unavailable but the system is GNU/Linux, and fail otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a decent improvement, yes. As long as not having gsed
on Darwin remains an error with an appropriate help message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current version in #683 certainly flags absence of gsed
on macOS as an error. It doesn’t provide help on how to install gsed
on macOS, since I believe that a Unix script shouldn’t provide assistance in installing certain software on particular operating systems and Homebrew should be so well known among developers using macOS that virtually all of them should know how to install gsed
(and the remaining few ones could find out from the first hit of a corresponding web search 🙂).
Since key contributions of this pull request have been carried over to #683 and further contributions could be carried over as well if necessary, I’m closing this pull request. |
Based on #683 with a few fixes.